Conversation
janbuchar
added
the
t-tooling
github-actions
bot
added
the
tested
janbuchar
force-pushed
the
recoverable-rendering-type-detection-results
branch
from
6271943 to
5f580ac
Compare
barjin
left a comment
barjin
left a comment
There was a problem hiding this comment.
There are some issues (not calling initialize), but it looks solid otherwise.
We might have different ideas about what this is for. I was imagining a fully generic "trait", that helps users with implementing serialize and deserialize methods for anything imaginable. The current implementation decisions seem to point more towards "simple" POJOs only.
This is fine by me, I'm just curious - have you tried (thought about) the former thing? Do you see any reasons why that wouldn't work?
packages/playwright-crawler/src/internals/utils/rendering-type-prediction.ts
Show resolved
Hide resolved
| "compilerOptions": { | ||
| "outDir": "./dist" | ||
| "outDir": "./dist", | ||
| "rootDir": "./src" |
There was a problem hiding this comment.
do you really need this (and the changes in the other tsconfig)? feels wrong to change it just for two packages
There was a problem hiding this comment.
I honestly don't know, but I added it to fix https://github.com/apify/crawlee/actions/runs/15419904093/job/43391934009
|
I spent quite a bit of time trying to make |
barjin
left a comment
barjin
left a comment
There was a problem hiding this comment.
Looks alright to me (aside from the TS edits, but I'll trust y'all on this). Again - thanks for the tests 🚀
packages/playwright-crawler/src/internals/utils/rendering-type-prediction.ts
Outdated
Show resolved
Hide resolved
| * across migrations or restarts. It manages the loading, saving, and resetting of state data, | ||
| * with optional persistence capabilities. | ||
| * | ||
| * The state is represented by a plain JavaScript object that can be serialized to and deserialized from JSON. |
There was a problem hiding this comment.
I suppose this is no longer completely true with custom serialize and deserialize methods, but there is IMO nothing wrong with not telling the users the whole truth, keeping them on the safe side.
There was a problem hiding this comment.
Well, "plain JavaScript object" is not exactly a legal term and we do accept stuff that can be (de)serialized from/to JSON, if only with the caveat that sometimes you have to write the serialization logic by hand.
3 participants
RecoverableStateclass with the hopes of using it for all persistent components (Statistics,SessionPool, ...), similarly to the Python port, in the futureRecoverableStateis utilized inRenderingTypePredictorto make its state persistent by default for better detection reliability after pauses and migrations